Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go straight to inbox on launch, when an account available #525

Merged
merged 16 commits into from
Feb 22, 2024

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Feb 17, 2024

Fixes: #516

Along the way, make some improvements to our abstractions for describing a page that needs a PerAccountStoreWidget (like MaterialAccountWidgetRoute). In particular:

  • extend them to cover the case where the account ID we want isn't found ambiently on the current context (and then use them more comprehensively, as we now can do);
  • have them tell PerAccountStoreWidget to use a loading placeholder that has a Scaffold and therefore a back button.

This doesn't particularly have a "page" nature; it'd work just
as well as one thing on a larger page, if it came to that.
I was rereading some of the code that uses these classes, and found
I had to reread their implementation to refresh myself on what they
each did.
In particular this provides a back button so that the user can
go try something else instead of waiting for the app to finish
getting data from the server.  (On Android the user could already
do so, using the system back gesture or button, but there's no
such thing on iOS.)
Now PerAccountStoreWidget has just two constructor call sites
in the app.  There's one for the flight shuttle in LightboxHero,
a rare example of a widget not belonging to any page; and then
the one in AccountPageRouteMixin covers every page in the app
that needs a per-account store.
This has the same effect, but gives us a more general form
of API to work with for more complex effects.

Effectively `onGenerateRoute` is a fallback for when a route name
isn't resolved by `home` or `routes`; and this version has the
same effect as the `home` (and omitted `routes`) we'd been providing.

I find the relevant area of the docs somewhat hard to follow, so to
see this in the implementation: the main effects of both `home` and
`onGenerateRoute` are implemented in [WidgetsApp._onGenerateRoute],
and the one other effect is in [WidgetsApp._usesNavigator].
The [MaterialWidgetRoute] part corresponds to `pageRouteBuilder`,
which is specified by the [MaterialApp] implementation.
The base class offers six methods, so we may as well provide
access to all six of them in the same way.
This mimics what the default implementation of onGenerateInitialRoutes
was doing, given that we never specify initialRoute and given our
implementation of onGenerateRoute.
This will let us use the global store in computing our arguments
for MaterialApp.
These tests had been relying on the fact that there wouldn't be any
PerAccountStoreWidget that reached the point of attempting to load
data for these accounts.  But that was true only because, after
opening a notification, the test code waited only for microtasks
(with `await null`), and not for a new frame (like `tester.pump()`).

That makes them brittle, as they'd break on an otherwise innocent
change to pump another frame.  Fortunately it's easy to fix.
@chrisbobbe
Copy link
Collaborator

This looks great! Merging.

It probably bumps the priority of #386, because the inbox page will be featured more prominently now. But even an empty inbox page has a title and a back button, so users shouldn't be totally stuck, and we do still clearly mark the app as beta.

@chrisbobbe chrisbobbe merged commit 256fa7e into zulip:main Feb 22, 2024
1 check passed
@gnprice gnprice deleted the pr-launch-inbox branch February 22, 2024 00:33
@gnprice
Copy link
Member Author

gnprice commented Feb 22, 2024

Thanks!

It probably bumps the priority of #386, because the inbox page will be featured more prominently now.

Good point, yeah, agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go directly on launch to inbox / home view
2 participants